Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #4551 Changed mock docks to use sphinx #4569

Merged
merged 14 commits into from Nov 1, 2018

Conversation

anubhavsinha98
Copy link

No description provided.

docs/faq.rst Outdated

.. Tip:: The library ``unittest.mock`` was introduced on python 3.3. On earlier versions install the ``mock`` library
from PyPI with (ie ``pip install mock``) and replace the above import::
Changed in version 1.6: This config value only requires to declare the top-level modules that should be mocked.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I should just link to the document of sphinx rather than pasting it and remove the earlier workaround.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stsewd changed!

@anubhavsinha98 anubhavsinha98 changed the title Issue #4551 Issue #4551 Changed mock docks to use sphinx Aug 24, 2018
docs/faq.rst Outdated

import sys
from unittest.mock import MagicMock
autodoc_mock_imports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could start this with something more explicit, like "You can use this feature included in autodoc to mock..." where the feature name would link to the sphinx's docs. Thanks for contributing btw :)

@anubhavsinha98
Copy link
Author

Hey @stsewd done some changes is it ok now?

@anubhavsinha98
Copy link
Author

Hey @stsewd check out the PR?

docs/faq.rst Outdated
@@ -65,27 +65,8 @@ I get import errors on libraries that depend on C modules

This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports.

You can mock out the imports for these modules in your ``conf.py`` with the following snippet::
You can use this feature included in autodoc to mock http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can put the name of the feature (and the link to it here) and end with "..to mock your dependencies."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing proper format for linking instead of naked link

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the naked link issue!

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, noted a couple spots to clean up the change

docs/faq.rst Outdated

from mock import Mock as MagicMock

If such libraries are installed via ``setup.py``, you also will need to remove all the C-dependent libraries from your ``install_requires`` in the RTD environment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should however keep this notice, as a lot of folks have mocked their dependencies in the past, but not performed this step, assuming mocking was enough.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agjohnson , can you suggest what notice I can create for the folks?
For eg. "There is a new feature "autodock_mock_imports" for mocking the dependencies, the earlier feature present was a complex one".
Please suggest!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be something like: "If you are mocking your dependencies using the mock library and such... "

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now check it @stsewd !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably just need to communicate that if you mock dependencies, in whatever fashion, that you aren't installing those dependencies either. The original copy just needs to be updated to match the new context i think.

docs/faq.rst Outdated
@@ -65,27 +65,8 @@ I get import errors on libraries that depend on C modules

This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports.

You can mock out the imports for these modules in your ``conf.py`` with the following snippet::
You can use this feature included in autodoc to mock http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing proper format for linking instead of naked link

@agjohnson agjohnson added this to the Better user documentation milestone Aug 27, 2018
docs/faq.rst Outdated
@@ -65,7 +65,7 @@ I get import errors on libraries that depend on C modules

This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports.

You can use this feature `autodoc_mock_imports`_ to mock your dependencies.
If you are mocking your dependencies using the mock library, then you can also use this feature `autodoc_mock_imports`_ to mock your dependencies.
Copy link
Member

@stsewd stsewd Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what Anthony mean, was to keep both paragraphs. The link to autodoc_mock_imports and the note about the previous usage of the mock lib. Sorry about the confusion.

@anubhavsinha98
Copy link
Author

Now check @stsewd and @agjohnson !

docs/faq.rst Outdated
@@ -87,6 +86,11 @@ Of course, replacing `MOCK_MODULES` with the modules that you want to mock out.

If such libraries are installed via ``setup.py``, you also will need to remove all the C-dependent libraries from your ``install_requires`` in the RTD environment.

If you are mocking your dependencies using the mock library, then you can also use this feature `autodoc_mock_imports`_ to mock your dependencies.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the autodoc feature is independent from the mock library and should be pointed first (as we want new users to use that method), the previous paragraph was ok btw, you only needed to keep the another paragraph

@anubhavsinha98
Copy link
Author

Check now @stsewd , I have marked the autodock feature on the top and then the previous one below.

@agjohnson agjohnson added the PR: work in progress Pull request is not ready for full review label Aug 28, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better. Just clarifying what we want to communicate to users.

docs/faq.rst Outdated
You can mock out the imports for these modules in your ``conf.py`` with the following snippet::
If you are mocking your dependencies without using the mock library, then you can use this feature `autodoc_mock_imports`_ to mock your dependencies.

.. _autodock_mock_imports: http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link label is wrong here, should be autodoc_mock_imports.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't recognize, I think it's okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo autodocK

docs/faq.rst Outdated
@@ -65,7 +65,13 @@ I get import errors on libraries that depend on C modules

This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports.

You can mock out the imports for these modules in your ``conf.py`` with the following snippet::
If you are mocking your dependencies without using the mock library, then you can use this feature `autodoc_mock_imports`_ to mock your dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we're wanting to say here is "The recommended method of mocking your libraries is to use the autodoc_mock_imports config option", not "if you are .."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I am changing it!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agjohnson I think its fine as it is the syntax for the removing the naked link.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Anthony says is that we should encourage people to use the autodoc_mock_imports method

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some small changes to get this merged. Thanks for the contribution!

@ericholscher ericholscher merged commit cb215c2 into readthedocs:master Nov 1, 2018
@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #4569 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4569   +/-   ##
=======================================
  Coverage   76.33%   76.33%           
=======================================
  Files         158      158           
  Lines        9968     9968           
  Branches     1258     1258           
=======================================
  Hits         7609     7609           
  Misses       2018     2018           
  Partials      341      341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants